Skip to content

Conversation

realsn0w
Copy link
Contributor

@realsn0w realsn0w commented Feb 22, 2025

the govet fieldalignment test currently fails on 32bit targets because it assumes uintptr always has a size of 8 bytes.
this pr (edited after some discussion) adds build tags to skip it on platforms where that is not the case.

Copy link

boring-cyborg bot commented Feb 22, 2025

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Feb 22, 2025

CLA assistant check
All committers have signed the CLA.

@ldez ldez self-requested a review February 22, 2025 18:23
@ldez
Copy link
Member

ldez commented Feb 22, 2025

Hello, in which context do you need to run our tests inside a 32-bit arch?

@ldez ldez added the waiting for: contributor feedback Requires additional feedback label Feb 22, 2025
@realsn0w
Copy link
Contributor Author

ran into this while working on the golangci-lint void package.
ci there runs a check stage after build for non-cross targets.
here is an example of a build failure caused by this.

also just in general, i feel like it would be nice if the tests work for all of the platforms you currently support / provide official binaries for (which currently includes armv6/7 and i386).

@ldez ldez added area: tests Continuous integration, tests and other checks and removed waiting for: contributor feedback Requires additional feedback labels Feb 22, 2025
@ldez ldez added this to the unreleased milestone Feb 22, 2025
@ldez
Copy link
Member

ldez commented Feb 22, 2025

We will not be able to maintain those tests because our CI doesn't use 32-bit arch (we will not add it inside our CI) and globally 32-bit arches are dead but 🤷

I don't understand why distribution packagers want to recompile everything and run tests: this is a Go program (statically linked) with no usage of CGO, not a C program dynamically linked.
A re-compilation has no real advantages in this context, and running tests is a bit of a waste of CPU cycles because we already run them, but I don't want to enter into a debate here.

I will just ask to remove 32-bit tests because they will never be run inside our CI.
We will keep the build tags.

@ldez ldez added the waiting for: contributor feedback Requires additional feedback label Feb 22, 2025
@realsn0w
Copy link
Contributor Author

alright, that sounds like a fair compromise.
thanks for the quick replies 🙂

@realsn0w realsn0w force-pushed the fixup/govet-fieldalignment-32bit branch from 1e5e980 to 263829a Compare February 22, 2025 20:37
@realsn0w realsn0w changed the title govet: add 32bit variant of fieldalignment test govet: skip fieldalignment test on 32bit platforms Feb 22, 2025
@ldez ldez removed the waiting for: contributor feedback Requires additional feedback label Feb 22, 2025
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ldez ldez merged commit 53d58e8 into golangci:master Feb 22, 2025
18 checks passed
@realsn0w realsn0w deleted the fixup/govet-fieldalignment-32bit branch February 23, 2025 10:09
@ldez ldez modified the milestones: unreleased, v1.64 Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: tests Continuous integration, tests and other checks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants